Skip to content

Fix issue #4501 Exception noise from OAuth and REST (API2 ) #4642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jul 7, 2025
Merged

Fix issue #4501 Exception noise from OAuth and REST (API2 ) #4642

merged 8 commits into from
Jul 7, 2025

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented Feb 22, 2025

Description (*)

See issue #4501. This PR added $shouldLog as an optional parameter in Mage_APi2_Exception, any code that extends it and overrides the constructor with the old signature would break.

Related Pull Requests

  • see OpenMage/magento-lts#<issue_number>

Fixed Issues (if relevant)

@github-actions github-actions bot added the Component: Api2 Relates to Mage_Api2 label Feb 22, 2025
kiatng
kiatng commented Feb 22, 2025
View reviewed changes
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@sreichel
Copy link
Contributor

any code that extends it and overrides the constructor with the old signature would break.

#4501 (comment)

How about to add _criticalNotLoggable() method and a new exception?

@kiatng
Copy link
Contributor Author

kiatng commented Feb 28, 2025

any code that extends it and overrides the constructor with the old signature would break.

#4501 (comment)

How about to add _criticalNotLoggable() method and a new exception?

I have thought about new exception class. I am hesitant because I do not think there is any custom code that would extend it, if there is, it's easy to fix. This avoid adding more code.

Hanmac
Hanmac previously approved these changes May 8, 2025
View reviewed changes
Copy link
Contributor

@Hanmac Hanmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay for now?

Another approach would be to check the log-level (like show them on debug/info?)

@sreichel sreichel added the bug label Jul 5, 2025
@sreichel sreichel removed the bug label Jul 5, 2025
sreichel
sreichel reviewed Jul 5, 2025
View reviewed changes
sreichel
sreichel reviewed Jul 5, 2025
View reviewed changes
sreichel
sreichel reviewed Jul 5, 2025
View reviewed changes
sreichel
sreichel reviewed Jul 7, 2025
View reviewed changes
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jul 7, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@sreichel
Copy link
Contributor

sreichel commented Jul 7, 2025

Already reviewed by @Hanmac .

@sreichel sreichel merged commit fb5acf6 into OpenMage:main Jul 7, 2025
22 checks passed
@sreichel sreichel deleted the issue4501_api2_exception_noise branch July 7, 2025 02:58
fballiano added a commit to MahoCommerce/maho that referenced this pull request Jul 7, 2025
@fballiano
OM PR 4642
679cb9f 
OpenMage/magento-lts#4642
@addison74
Copy link
Contributor

This one has a PHPstan error. Did you check it?

@sreichel
Copy link
Contributor

sreichel commented Jul 7, 2025

All lights green here?

@Hanmac
Copy link
Contributor

Hanmac commented Jul 8, 2025

@addison74 there was an outdated PHPstan error, that got fixed with a later commit

hirale pushed a commit to hirale/magento-lts that referenced this pull request Jul 11, 2025
@kiatng @sreichel @hirale
Fix issue OpenMage#4501 Exception noise from OAuth and REST (API2 ) (O…
8478140 
…penMage#4642)

* Fix issue OpenMage#4501 Exception noise from OAuth and REST (API2 )

* Update app/code/core/Mage/Api2/Model/Resource.php

* Update app/code/core/Mage/Api2/Exception.php

Co-authored-by: Sven Reichel <[email protected]>

* Update app/code/core/Mage/Api2/Exception.php

Co-authored-by: Sven Reichel <[email protected]>

* Update app/code/core/Mage/Api2/Exception.php

Co-authored-by: Sven Reichel <[email protected]>

* Update app/code/core/Mage/Api2/Exception.php

---------

Co-authored-by: Sven Reichel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sreichel sreichel sreichel approved these changes

@Hanmac Hanmac Hanmac left review comments

Assignees
No one assigned
Labels
Component: Api2 Relates to Mage_Api2 enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exception noise from OAuth and REST (API2 )
4 participants
@kiatng @sreichel @addison74 @Hanmac